-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow almost all printable ASCII characters in environment variables #4373
Allow almost all printable ASCII characters in environment variables #4373
Conversation
HirazawaUi
commented
Dec 21, 2023
- One-line PR description: Allow special characters in environment variables
- Issue link: Allow almost all printable ASCII characters in environment variables #4369
- Other comments:
36476b0
to
dfacff4
Compare
dfacff4
to
846cf89
Compare
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
846cf89
to
0079aee
Compare
/cc |
aeac2c2
to
b75f5bb
Compare
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
- "@HirazawaUi" | ||
owning-sig: sig-node | ||
participating-sigs: | ||
- sig-api-machinery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant to SIG Security?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the description, I assume this has nothing to do with SIG Security?https://github.com/kubernetes/community/tree/master/sig-security#security-special-interest-group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to get a consultation - security holes are rarely where reasonable people expect them to be. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @IanColdwater @tabbysable
keps/sig-node/4369-allow-special-characters-environment-variable/kep.yaml
Outdated
Show resolved
Hide resolved
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Outdated
Show resolved
Hide resolved
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
b833deb
to
950107c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I am OK with this proposal. Caution is warranted, but reasonable caution, IMO.
@liggitt or @SergeyKanzhelev objections or further consultations you want?
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
|
||
## Proposal | ||
|
||
* Implements relaxed validation at the top level validation method when validating API create requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you detail what the new validation will be? Is it "anything but '='
or is it something more precise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to being explicit here. The summary says Allow all printable ASCII except '='
. Is that well-defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The summary says
Allow all printable ASCII except '='
. Is that well-defined?
I wrote a demo to verify the ASCII characters that can be set as environment variables
package main
import (
"fmt"
"os"
)
func setEnvVars() {
for i := 0; i < 128; i++ {
envKey := "a" + string(i)
if err := os.Setenv(envKey, "abc"); err != nil {
fmt.Println(i)
fmt.Printf("set character %s to env name failed \n", envKey)
}
}
}
func main() {
setEnvVars()
}
it results is
➜ ~ ./envvar
0
set character a to env name failed
61
set character a= to env name failed
It looks like all ASCII characters can be set as environment variables except for NUL
with serial number 0 and =
with serial number 61.... But I'd like to check if the CRI API restricts them first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is the main open issue, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the test results in #4373 (comment), I think we can constrain the characters to characters 33-126 in ASCII. I will add some regex to KEP, and elaborate it in more detail.
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
- "@HirazawaUi" | ||
owning-sig: sig-node | ||
participating-sigs: | ||
- sig-api-machinery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to get a consultation - security holes are rarely where reasonable people expect them to be. :)
keps/sig-node/4369-allow-special-characters-environment-variable/kep.yaml
Outdated
Show resolved
Hide resolved
reviewers: | ||
- "@liggitt" | ||
- "@thockin" | ||
- TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyKanzhelev or someone else from sig-node may want to look at it.
|
||
### Risks and Mitigations | ||
|
||
Relaxed validation can break upgrade and rollback scenarios, but our use of feature gate to control whether it's enabled or not will make it a manageable risk, with the user having the autonomy to choose whether or not to enable it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature gates aren't intended to be a long-term configuration option. How is the risk managed as the feature gate graduates to GA and is enabled by default without user intervention, or is locked on and can no longer be disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume it's like any other - locked once GA. Any reason not to follow this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion is the same as @thockin, it should be the same as other feature gates, if we think it has risks when upgrade and rollback scenarios, we can let the beta time duration be long enough
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Show resolved
Hide resolved
|
||
## Proposal | ||
|
||
* Implements relaxed validation at the top level validation method when validating API create requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to being explicit here. The summary says Allow all printable ASCII except '='
. Is that well-defined?
keps/sig-node/4369-allow-special-characters-environment-variable/README.md
Outdated
Show resolved
Hide resolved
|
||
* Add a test to `test/e2e/common/secret.go` to test that the special characters in secret are consumed by the environment variable. | ||
|
||
* Add a test to `test/e2e/common/expansion` to test environment variable can contain special characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add details about what this will exercise and that it checks all the way into the container (e.g. from kubernetes/kubernetes#53201 (comment))
e2e tests demonstrating we can actually construct an environment with envvar names at the edges of what we support and it makes it into the container successfully (envvar names containing newlines/whitespace, unprintable characters, multi-byte characters, "difficult" characters like
'"*?;/\()&$
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any CRI-specific (containerd vs crio vs docker vs ...) or OS-specific (linux or windows) oddities about support for envvars with weird characters in the names? do CRI implementations have to make adjustments to support this? does the CRI API specify allowed envvar names or is it arbitrary / opaque
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do CRI implementations have to make adjustments to support this? does the CRI API specify allowed envvar names or is it arbitrary / opaque
Let me do some research...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a demo to test envvar name support for different container runtimes that allow all printable ASCII characters as envvar names, even "=" is allowed, They have more relaxed restrictions on envvar than k8s.
ref: https://github.com/HirazawaUi/verfiy-container-env
# The most recent milestone for which work toward delivery of this KEP has been | ||
# done. This can be the current (upcoming) milestone, if it is being actively | ||
# worked on. | ||
latest-milestone: "v1.30" | ||
|
||
# The milestone at which this feature was, or is targeted to be, at each stage. | ||
milestone: | ||
alpha: "v1.30" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure these are right for a provisional
KEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will modify it if it is not implemented in 1.30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provisional KEPs are not supposed to have code merging, versus implementable
The idea being, provisional KEPs may have part of an idea merged to iterate on but aren't agreed to implement yet. An implementable KEP would be expected to have stronger approval and would have a target release for implementing. https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/0000-kep-process/README.md#kep-workflow
@thockin Sorry for the delay, has already been updated, my initial thought was to update after all the comments have been processed over the weekend |
d5a17e0
to
c1ef3d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
* Strict validation follows the current design, which only allows envvar names passed the regular expression `[-._a-zA-Z][-._a-zA-Z0-9]*`. | ||
|
||
- Relaxed validation | ||
* Relaxed verification allows all ASCII characters with numbers in the range 33-126 as envvar name, and its regular expression is `^[!-~]{1,}$`, matches string that contains ASCII characters from the `!` to the `~` and is at least 1 in length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe describe this (or implement it) as unicode.IsPrint(r) && r < unicode.MaxASCII
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to filter out "=" from all printable ASCII characters, it will be easier to use regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I prefer unicode.IsPrint(r) && r < unicode.MaxASCII
to a range or regex is that it's not reasonable to expect people to keep the ASCII range in their heads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all right, I agree with your opinion.
- "@HirazawaUi" | ||
owning-sig: sig-node | ||
participating-sigs: | ||
- sig-api-machinery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @IanColdwater @tabbysable
c1ef3d7
to
fd39ea5
Compare
@thockin I updated the documentation to weaken "printable ASCII characters" and emphasize using the range of decimal numbers in ASCII to describe it, may you take a look at it again? |
f3056fc
to
8131cc9
Compare
8131cc9
to
53c565c
Compare
Before, I mistakenly thought that the |
I'm still LGTM to proceed on this. /lgtm |
Since it's close to the enhancement freeze, I'll contact the maintainers of sig node & sig security on slack to review it (if they're interested). |
@liggitt Do you have any other thoughts on it? |
This PR also needs to add a file in keps/prod-readiness to trigger PRR. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HirazawaUi, mrunalp, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@HirazawaUi You still need to trigger PRR or this will not move forward |
all right. |
|
||
### Monitoring Requirements | ||
|
||
- N/A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not N/A when going beta, GA. ok for alpha
# The most recent milestone for which work toward delivery of this KEP has been | ||
# done. This can be the current (upcoming) milestone, if it is being actively | ||
# worked on. | ||
latest-milestone: "v1.30" | ||
|
||
# The milestone at which this feature was, or is targeted to be, at each stage. | ||
milestone: | ||
alpha: "v1.30" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provisional KEPs are not supposed to have code merging, versus implementable
The idea being, provisional KEPs may have part of an idea merged to iterate on but aren't agreed to implement yet. An implementable KEP would be expected to have stronger approval and would have a target release for implementing. https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/0000-kep-process/README.md#kep-workflow